fix(e5): distinguish proved cloud downloads from uploads - #891

[SkillSpector Review]

Value and readiness

The direction proof is conservative and preserves per-occurrence upload locations. Reviewed code is approvable, although the branch currently conflicts with main and needs reconciliation before merging.

Material findings

No required code changes found. The proof rejects compound context, runtime operands, lookup rebinding, streams, unknown flags and host-language commands. Relative destinations remain conservative after directory changes. No prior blocker threads.

PIC tradeoffs

Only simple AWS/GCS transfers qualify; the 4096-character complete-prefix requirement deliberately limits false-positive relief in long scripts. SDK detection is unchanged.

Verification and gaps

Reviewed exact head 8768a630dcd98ec3ae26db8593165b3cfd1152ca, the full diff, pinned source, description, tests, and all prior review/comment threads. Read the full diff and pinned supply-chain helpers used for logical commands, compound-context ownership and shell tokens. Tests exercise uploads, setup rebinding, streams, CLI strict gates and source locations. No checks are attached to this exact head. Cited hosted run 39537680751 succeeded at fa24619, which is older than this head and is not exact-head CI evidence. Static review only; no contributor code, tests, installs, or external services were executed.

[SkillSpector Review]

Value and readiness

Ready for maintainer review. The per-occurrence proof removes E5 false positives for literal cloud downloads while keeping upload evidence and locations. The prior marked approval had no code blocker; its merge-conflict limitation is resolved at this MERGEABLE head.

Material findings

No required changes found. static_patterns_data_exfiltration.py:190-284 requires complete bounded preceding context, a supported shell host, literal operands, recognized non-deleting flags and a local non-stream destination. Unknown setup, earlier transfers, runtime expansion, command rebinding/compound syntax, and relative destinations after directory changes retain E5. The occurrence loop at lines 664-671 leaves SDK patterns and upload locations intact. Reviewed all 395 added test lines, including strict CLI exits, nearest occurrence locations, lookup/setup changes, context/length boundaries and the shell helpers, streams and host languages. Existing E1 parsing is retained.

PIC tradeoffs

The complete-prefix 4,096-character bound and small preamble/option allowlists deliberately limit false-positive relief. SDK detection and scoring policy remain unchanged. No additional PIC decision identified.

Verification and gaps

Read the complete three-file diff, exact-head analyzer, referenced logical-command/token/compound-context helpers, all marked reviews and comments/threads, and added regression assertions. Exact-head CI run 41516369359: changes, lint, OpenCode TypeScript Tests, DCO and docker-smoke succeeded. test-unit reports 18,047 passed and 15 failed; all failed test IDs exactly match main e14c450b run 41513800480, in PowerShell/quote ownership. There is no additional failing test ID; the overall suite remains red and GitHub reports UNSTABLE. No required-check names are configured. Static review only; no contributor code, tests, imports, builds, setup, regex probes, scanner runs or cloud transfers were executed.

Reviewed head: bdc072d53b6cf66e1b05406f04ed0262ca8c17bd.

[SkillSpector Review]

Value and readiness

Re-review after the merge with main. The PR's own changes are unchanged since the last approved head 95df851b, and the E5 loop now runs the per-occurrence download proof inside main's bounded static_runner.iter_pattern_matches, so the bounded matching is kept. The earlier approvals had no open points. One P2 in the preamble scan, present at the earlier approved heads as well, should be fixed before merging.

Material findings

• P2 src/skillspector/nodes/analyzers/static_patterns_data_exfiltration.py:227 — splitlines() treats a form feed, lone CR or other non-newline break as a line end, so a # after it hides an aws() rebinding that the shell runs, and the download keeps its exemption.

PIC tradeoffs

The 4,096-character complete-prefix bound and the short option/preamble allowlists still deliberately limit false-positive relief. The proof is per file and treats a #! line as a comment, so a shebang such as #!/usr/bin/env -S PATH=./tools:/usr/bin:/bin sh that points aws at a bundled helper is not seen. Maintainers should either accept that or accept only plain interpreter shebangs.

Verification and gaps

Read the full three-file diff at this head and compared it with the diff at 95df851b: no PR hunk was lost in the merge and E5 is still scanned once. Traced _is_literal_cloud_download through the shared logical-command, compound-context and tokenizer helpers, the runner's bounded matcher, window/view handling and file decoding. Shell line-ending behaviour was checked with harmless stand-alone scripts, and splitlines()/regex behaviour with stdlib re-typings of the expressions. Hosted CI at this head: all six checks succeeded. Static review only: no contributor code, tests, imports, builds or scanner invocations were executed.

Reviewed head: 09427e2a32ef5452ef8eff49c8689380de2e25f9.

[SkillSpector Review]

Decision: Changes Requested

This replaces my earlier approval. The P2 preamble-scan finding needs a fix before merge. str.splitlines() treats a form feed, a lone CR and other non-newline breaks as line ends, so a # after one of them is skipped as a comment, although sh/bash only end a line at \n. A preceding aws() rebinding stays hidden, and the later aws s3 cp s3://... /tmp/... keeps its download exemption, so E5 is dropped where main reports it. Rejecting non-\n breaks in content[:offset] (the rule _sc2_logical_command already applies) and adding those variants to test_visible_command_redefinition_keeps_cloud_finding would resolve it. The rest of the review is unchanged. The commits since 09427e2a only merge main and leave the E5 analyzer and its tests unchanged.

Reviewed head: 423cca7050c2a347882764f1cd9e269a3d4dc91d.

[SkillSpector Review]

Decision: Approved

This replaces my earlier change request. The P2 preamble-scan finding is fixed on this branch:

• 23439d07 ends preamble comments only at a newline. Any other break before the transfer drops the download exemption, the same rule _sc2_logical_command applies. That covers a form feed, vertical tab, lone CR, \x1c-\x1e, NEL, U+2028 and U+2029. The preamble is now split on \n rather than with splitlines(). test_non_newline_break_cannot_hide_a_rebinding_as_a_comment hides an aws()/gsutil() rebinding behind each separator and expects E5.
• 1786e323 keeps the exemption after a leading #! line only when it names a plain sh, bash, dash, ksh or zsh, directly or through /usr/bin/env, optionally with -e/-u. Everything else keeps E5, since it can rebind the transfer command or load setup code before the transfer runs: env -S assignments such as PATH=, BASH_ENV= or ZDOTDIR=, interpreter options like --rcfile, relative interpreters and non-shell interpreters. test_unsupported_shebang_keeps_cloud_finding covers these cases. The supported-preamble test now includes shebangs and CRLF line endings and still keeps the exemption.

The rest of the change is as reviewed earlier. The full unit suite passed locally (2,355 tests), and all hosted checks succeeded at this head.

Reviewed head: 1786e323f0b7e313d9ac06183176cf7d6fe9cd60.